This would make a good test case, though. We have existing tests that register services for tests using an extra class loader:

https://github.com/junit-team/junit5/blob/16c6f72c1c728c015e35cb739ea75884f19f990c/platform-tests/src/test/java/org/junit/platform/launcher/core/LauncherFactoryTests.java#L353-L366

We could generalize and move that method to a test utility class (e.g. in junit-jupiter-api/src/testFixtures) so it can be reused here.

Another possible integration test could be inspired by #3605.

We could generalize and move that method to a test utility class (e.g. in junit-jupiter-api/src/testFixtures) so it can be reused here.

@marcphilipp fine if I do it in a separate PR? Mostly to keep the size of this one under control 🙃

Raised #4544.

This class should return false from canConvert when source is null, shouldn't it?

It's possible that you commented before I refactored the Converter hierarchy, so what I'm about to say might not align with what you had in mind 🙂

Currently, LocaleConverter extends TypedConverter, and I assume the latter should maintain support for null values, similar to TypedArgumentConverter.

However, for the sake of this test, LocaleConverter could also extend Converter directly and have a more fine-grained canConvert implementation, given that the underlying Locale.forLanguageTag API is not supposed to accept null values.

Anyway, I think this test lost its value as DefaultConverter uses Locale.forLanguageTag natively (#4751), so I'll probably rewrite it differently or replace it with a more meaningful one.

I think this is a NullAway bug. I'll compose a small reproducer and raise an issue with the project.

Hmm, that is weird. How does NullAway know about the nullability of the Function type parameters of Optional.map? Maybe it doesn't?

I'm still playing with it to understand it better. For example, the error is slightly different with a lambda:

public Optional<Class<?>> getWrapperType() {
	return Optional.ofNullable(type).map(value -> ReflectionUtils.getWrapperType(value));
}

warning: [NullAway] passing @Nullable parameter 'value' where @NonNull is required
		return Optional.ofNullable(type).map(value -> ReflectionUtils.getWrapperType(value));
		                                                                             ^
    (see http://t.uber.com/nullaway )

I would have expected NullAway to infer that value is non-nullable, but maybe it has no way to do that.

It looks to me like it thinks Optional.map is typed as

<U> Optional<U> map(Function<? super @Nullable T, ? extends @Nullable U> mapper)

when it should be

<U> Optional<U> map(Function<? super @NonNull T, ? extends @Nullable U> mapper)

Is this extra indirection worth it?

The advantage I see is that classes extending this one will have a simpler surface, without ConversionContext appearing in the code.

Also, after the updates mentioned at #4219 (comment), this will prevent null source.

I guess the constructors need to be public so they can be used in unit tests for Converter implementations, right?

I kept them public for any code invoking ConversionSupport.convert(), which is not only DefaultArgumentConverter but potentially user code as ConversionSupport is a public API.

Should we pass source here as well? In other words, could a Converter only be able to convert S only partially (for example, excluding null)?

I initially tried this direction but ended up with class cast exceptions due to Converter having type parameters, so I eventually removed it.

I also checked how Spring does it in ConditionalConverter, and the strategy seemed to be the same, so I thought this was a common pitfall 🙂